Skip to content

chart(breaking change): update config key to enable ingress #2349

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 12, 2024
Merged

chart(breaking change): update config key to enable ingress #2349

merged 1 commit into from
Aug 12, 2024

Conversation

@VietND96
Copy link
Member

@VietND96 VietND96 commented Aug 11, 2024 •
edited
Loading

User description

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

chart(breaking change): update config key to enable ingress

Motivation and Context

Config keys are changed as below

ingress.enabled changes to ingress.enableWithExistingController: Enable or disable ingress resource (without installing Ingress NGINX Controller subchart)

Now ingress.enabled to used to enable ingress resource. Implies installing Ingress NGINX Controller

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Tests, Documentation

Description

  • Enhanced release notes generation script to include Helm chart version and Docker Hub link.
  • Added --credential yes flag to docker run command in chart setup script.
  • Introduced RENDER_HELM_TEMPLATE_ONLY variable for conditional execution in chart test script.
  • Added new helper functions for ingress and monitoring enablement checks in Helm templates.
  • Updated GitHub workflows to include options for skipping image build and push, and added disk space cleanup steps.
  • Updated Makefile with new targets for rendering Helm templates and updated chart test targets.
  • Updated conditions for dependencies in Helm chart to include new enablement flags.
  • Updated README with new ingress and tracing configurations.
  • Updated ingress enablement checks in Helm templates to use new helper functions.
  • Added new flags for ingress and monitoring configurations in values.yaml.

Changes walkthrough 📝

Relevant files
Enhancement
14 files
generate_release_notes.sh
Enhance release notes with Helm chart version and Docker Hub link

generate_release_notes.sh

  • Added Helm chart version to release notes.
  • Updated Docker images section with a link to Docker Hub.
    		•  +4/-1     
    chart_setup_env.sh
    Add credential flag to multiarch/qemu-user-static docker run command

    tests/charts/make/chart_setup_env.sh

  • Added --credential yes flag to docker run command for
    multiarch/qemu-user-static.
    		•  +1/-1     
    chart_test.sh
    Add conditional execution for Helm template rendering       

    tests/charts/make/chart_test.sh

  • Introduced RENDER_HELM_TEMPLATE_ONLY variable for conditional
    execution.
  • Added conditions to skip certain steps based on
    RENDER_HELM_TEMPLATE_ONLY.
    		•  +17/-6   
    _helpers.tpl
    Add helper functions for ingress and monitoring enablement

    charts/selenium-grid/templates/_helpers.tpl

  • Added new helper functions for ingress and monitoring enablement
    checks.
    		•  +16/-2   
    deploy.yml
    Add option to skip image build and push in deploy workflow

    .github/workflows/deploy.yml

  • Added skip-build-push-image input to skip image build and push steps.
  • Added step to render chart templates.
    		•  +23/-8   
    docker-test.yml
    Add disk space cleanup step in docker-test workflow           

    .github/workflows/docker-test.yml

    • Added step to free disk space on Ubuntu before tests.
    		 +5/-0     
    helm-chart-test.yml
    Add disk space cleanup step in helm-chart-test workflow   

    .github/workflows/helm-chart-test.yml

    • Added step to free disk space on Ubuntu before tests.
    		 +5/-0     
    rerun-failed.yml
    Add write-all permissions to rerun-failed workflow             

    .github/workflows/rerun-failed.yml

    • Added permissions: write-all to the workflow.
    		 +2/-0     
    Makefile
    Add chart_render_template target and update chart test targets

    Makefile

  • Added chart_render_template target for rendering Helm templates.
  • Updated chart test targets to include TEMPLATE_OUTPUT_FILENAME.
    		•  +9/-0     
    Chart.yaml
    Update dependency conditions with new enablement flags     

    charts/selenium-grid/Chart.yaml

  • Updated conditions for dependencies to include new enablement flags.
    		•  +4/-4     
    NOTES.txt
    Update ingress enablement check in NOTES.txt                         

    charts/selenium-grid/templates/NOTES.txt

    • Updated ingress enablement check to use new helper function.
    		 +1/-1     
    ingress.yaml
    Update ingress enablement check in ingress.yaml                   

    charts/selenium-grid/templates/ingress.yaml

    • Updated ingress enablement check to use new helper function.
    		 +1/-1     
    values.yaml
    Add new flags for ingress and monitoring configurations   

    charts/selenium-grid/values.yaml

  • Added enableWithExistingController flag for ingress.
  • Added enabledWithExistingAgent flag for monitoring.
    		•  +15/-3   
    base-auth-ingress-values.yaml
    Add enabled flag for ingress in base-auth-ingress-values.yaml

    tests/charts/ci/base-auth-ingress-values.yaml

    • Added enabled flag for ingress.
    		 +1/-1     
    Documentation
    1 files
    README.md
    Update README with new ingress and tracing configurations

    charts/selenium-grid/README.md

  • Updated examples and descriptions to reflect new ingress and tracing
    configurations.
    		•  +7/-5     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @VietND96
    chart(breaking change): update config key to enable ingress
    c9f690b 
    Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Aug 11, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Ingress Condition
    The condition for enabling ingress has been updated to check both ingress.enabled and ingress.enableWithExistingController. Ensure that this logic correctly handles all intended use cases and does not unintentionally enable or disable ingress.

    Default Values
    The default values for ingress.enabled and ingress.enableWithExistingController have been changed. This might affect existing deployments that rely on the previous default settings. Ensure that these changes are clearly documented and communicated to users to avoid breaking changes.

    Conditional Steps
    Several steps in the GitHub Actions workflow now include conditions based on the skip-build-push-image input. Verify that these conditions are correctly set to avoid skipping necessary build steps unintentionally.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Aug 11, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a condition to prevent unintended deployments in GitHub Actions workflow

    Add a condition to check if the skip-build-push-image input is 'true' before running
    the 'Deploy new images' job to prevent unintended deployments when image building
    and pushing is skipped.

    .github/workflows/deploy.yml [136-143]

     - name: Deploy new images
+  if: github.event.inputs.skip-build-push-image != 'true'
   uses: nick-invision/retry@master
   with:
     timeout_minutes: 20
     max_attempts: 3
     retry_wait_seconds: 120
     command: VERSION="${GRID_VERSION}" BUILD_DATE=${BUILD_DATE} make release
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion prevents unintended deployments in the GitHub Actions workflow, addressing a possible issue that could lead to incorrect deployments.

    		9
    Security
    Ensure proper quoting of variables in shell scripts to prevent security issues

    Ensure that the NAMESPACE variable is properly quoted to prevent globbing and word
    splitting issues, which can lead to unexpected behavior or security vulnerabilities.

    generate_release_notes.sh [48]

    -echo "### Published Docker images on [Docker Hub](https://hub.docker.com/u/${NAMESPACE})" >> release_notes.md
+echo "### Published Docker images on [Docker Hub](https://hub.docker.com/u/\"${NAMESPACE}\")" >> release_notes.md
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Properly quoting variables in shell scripts is important for preventing security issues and unexpected behavior. This suggestion addresses a potential security concern.

    		8
    Best practice
    Ensure consistent credential handling across architectures in Docker commands

    Add the missing --credential yes option to the docker run command for the non-amd64
    architecture cases to ensure consistency in handling credentials across different
    architectures.

    tests/charts/make/chart_setup_env.sh [35-36]

    -docker run --rm --privileged aptman/qus -- -r ;
-docker run --rm --privileged aptman/qus -s -- -p
+docker run --rm --privileged aptman/qus -- -r --credential yes;
+docker run --rm --privileged aptman/qus -s -- -p --credential yes
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding the --credential yes option ensures consistent credential handling across different architectures, which is a good practice for maintaining uniform behavior.

    		7
    Maintainability
    Use a separate variable for URLs to ensure proper handling of special characters and dynamic content

    Replace the hardcoded selenium-grid-${chart_version} with a variable that includes
    the full URL, ensuring it is dynamically generated and properly escaped to handle
    special characters in the version string.

    tests/charts/make/chart_test.sh [60]

    -echo "### Published Helm chart version [selenium-grid-${chart_version}](https://github.com/${AUTHORS:-"SeleniumHQ"}/docker-selenium/releases/tag/selenium-grid-${chart_version})" >> release_notes.md
+full_chart_url="https://github.com/${AUTHORS:-"SeleniumHQ"}/docker-selenium/releases/tag/selenium-grid-${chart_version}"
+echo "### Published Helm chart version [selenium-grid-${chart_version}](${full_chart_url})" >> release_notes.md
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a separate variable for URLs improves maintainability and ensures proper handling of special characters, though the improvement is minor.

    		6

    @VietND96 VietND96 merged commit 91233c4 into SeleniumHQ:trunk Aug 12, 2024
    @VietND96 VietND96 added this to the 4.23.1 milestone Aug 12, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    documentation enhancement Review effort [1-5]: 4 tests

    Projects

    None yet

    Milestone

    4.23.1

    Development

    Successfully merging this pull request may close these issues.

    1 participant

    @VietND96